Java bindings for LP/MIP/QP - #1524
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesJava bindings
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mlubin
left a comment
There was a problem hiding this comment.
I saw the PR is closed, sending my comments as I had them already written up.
|
|
||
| extern "C" { | ||
|
|
||
| cuopt_int_t cuOptLoadParametersFromFile(cuOptSolverSettings settings, const char* path); |
There was a problem hiding this comment.
We should discuss merging these extensions into the C API.
| @@ -0,0 +1,1367 @@ | |||
| /* | |||
| * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
I'd recommend avoiding a test dependency on the python interface. The java interface should stand on its own.
| @@ -0,0 +1,25 @@ | |||
| # cuOpt Java bindings (beta) | |||
|
|
|||
| This directory is an isolated, customer-specific beta module for the cuOpt | |||
There was a problem hiding this comment.
Is this how we want to ship it?
There was a problem hiding this comment.
We would want to follow cuvs and try to publish to maven https://mvnrepository.com/artifact/com.nvidia.cuvs/cuvs-java
|
Sorry that was an accident. Reopening. |
| @@ -0,0 +1,28 @@ | |||
| /home/cbrissette/cuopt/java/cuopt/src/main/java/com/nvidia/cuopt/linearprogramming/BatchSolve.java | |||
There was a problem hiding this comment.
Do we need these files ? may be we can delete all the run time files so developers can concentrate on main parts.
ramakrishnap-nv
left a comment
There was a problem hiding this comment.
Focused review on APIs and shipping (vs how cuvs ships Java).
APIs: the surface is broad and, pleasingly, closely in sync with the Python API — the algebraic Problem layer matches Python's (camelCase) modeling methods almost 1:1, and DataModel maps cleanly (snake_case→camelCase). A few parity gaps and Java-idiom nits are noted inline.
Shipping: the main blockers — don't commit target/, and wire the build into CI/release the way cuvs does (ci/build_java.sh/ci/test_java.sh, dependencies.yaml java key, workflow jobs, version marker, docs toctree).
Non-blocking review comments below.
| } | ||
|
|
||
| /** Return true for maximize and false for minimize, matching Python get_sense(). */ | ||
| public boolean getSense() { |
There was a problem hiding this comment.
Parity note (not a rename request): getSense() correctly matches the Python DataModel.get_sense() (bool, True=maximize) — good. Two parity gaps vs Python though: (1) Python puts set_initial_primal_solution/set_initial_dual_solution on DataModel, whereas here they're on SolverSettings; (2) Python DataModel also exposes getters this class seems to lack: get_quadratic_objective_{values,indices,offsets}, get_variable_names/get_row_names, get_objective_name/get_problem_name, get_ascii_row_types.
| resetSolvedValues(); | ||
| } | ||
|
|
||
| public Object getObjective() { |
There was a problem hiding this comment.
getObjective() returns Object — callers must downcast. Prefer a typed return (or overloads). Same for SolverSettings.getTypedParameter() / getMipCallbacks(). (The modeling API otherwise tracks the Python Problem layer 1:1 — nice.)
| public enum ProblemCategory { | ||
| LP(0), | ||
| MIP(1), | ||
| IP(2); |
There was a problem hiding this comment.
We should deprecate IP across the whole code base.
chris-maes
left a comment
There was a problem hiding this comment.
Thanks for adding the JAVA API. Let's make sure that capitalization is consistent before merging.
| ``VariableType.SEMI_CONTINUOUS``; and | ||
| * ``ProblemCategory`` for the native problem classification. | ||
|
|
||
| ``ProblemCategory.IP`` is deprecated. Java normalizes the legacy native IP |
There was a problem hiding this comment.
No need to include or mention deprecated API. Remove all references to IP.
| * ``ConstraintSense.LE``, ``ConstraintSense.GE``, and ``ConstraintSense.EQ``; | ||
| * ``VariableType.CONTINUOUS``, ``VariableType.INTEGER``, and | ||
| ``VariableType.SEMI_CONTINUOUS``; and | ||
| * ``ProblemCategory`` for the native problem classification. |
There was a problem hiding this comment.
What is ProblemCategory used for? Could we get away with just isMIP()?
|
|
||
| ``Constraint`` provides ``getSense``, ``getRHS``, ``getCoefficient``, | ||
| ``getLinearExpression``, ``getQuadraticExpression``, ``isQuadratic``, | ||
| ``computeSlack``, ``getSlack``, and ``getDualValue``. |
There was a problem hiding this comment.
Is computeSlack needed. Let's provide the slack inside the C++ engine.
|
|
||
| ``Solution`` implements ``AutoCloseable`` and exposes: | ||
|
|
||
| * ``getPrimalSolution``, ``getDualSolution``, and ``getReducedCost``; |
There was a problem hiding this comment.
I'm not sure we want getPrimalSolution, getDualSolution and getReducedCost? These should be accessed via the constraints and variables.
| * ``getSolveTime`` and ``getProblemCategory``; and | ||
| * ``getVars`` when variable names are available. | ||
|
|
||
| LP solutions additionally expose ``getLPStats``. ``LPStats`` contains primal |
There was a problem hiding this comment.
Let's add attributes instead of stats.
| MPS and Errors | ||
| -------------- | ||
|
|
||
| ``Problem.read`` and ``Problem.readMPS`` support MPS/QPS parsing, including a |
|
@chris-maes thanks for the review — replying to the C API comments in one place since they're connected. Two facts first, because I think one of them changes the pictureThere is no solution-scoped attribute API today. All five attribute getters are None of these stats were reachable by any route before this PR. Checking Why we think they're worth supportingThe data already exists on
Two more general points. Python already exposes all of this by binding to the C++ structs through Cython, so today the C ABI is a second-class path — that asymmetry is what #1202 was filed about. And users arriving from Gurobi or CPLEX expect Where we agree, and what we'd proposeAttributes are the right shape for these. All eleven are scalar ints and floats, which is exactly what the attribute model is for. There's also a maintenance argument specific to the bindings: cuOpt already generates What we'd suggest is a shape-based rule rather than attributes everywhere, since it's the rule the C API already follows:
The third case is why Happy to convert the two bulk Conceded
The One question backOn the parameter calls ( |
Reworks the C API surface this PR adds, following review. Seven functions become two. Statistics are now read with cuOptGetSolutionIntAttribute and cuOptGetSolutionFloatAttribute using CUOPT_SOLUTION_ATTR_* selectors, in place of cuOptGetLPSolverStats and cuOptGetMIPSolverStats. This matches how problem data is already read, and it means a future statistic is a new constant rather than a new exported symbol, so existing callers keep working without a relink. It also costs the bindings nothing: CuOptConstants.java is generated from constants.h, so a new selector reaches Java with no hand-written code, where a new function needs a JNI entry point, a native declaration, and a wrapper. Selectors are numbered in their own range so a problem selector passed to a solution accessor is rejected rather than silently read, and LP selectors require an LP solution while MIP selectors require a MIP solution. cuOptSolutionIsMIP is removed as redundant. Solution is constructed with the category of the problem it was solved for, so it already knows whether it came from the MIP solver, and a C caller holding the problem handle has cuOptIsMIP. The four parameter-file and parameter-enumeration entry points are removed rather than promoted. Nothing needs them to build, solve, or read a solution, and one of them exposed the hyperparameters concept through a public header, which it should not. The capability is recorded in NVIDIA#1705 so it is deferred rather than lost. On the Java side this drops getSolverSettingNames, toDict, and the settings-file methods; setOptimalityTolerance now names the six LP tolerances explicitly instead of discovering them by matching parameter names, which is also less fragile than the previous filter. Verified: 69/69 C API gtests, including new tests that the accessors reject a mismatched value type, the wrong solver's selectors, unknown selectors, and null arguments; 26/26 Java tests; all pre-commit hooks. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
clang-format widens the value column to the longest macro name in the block. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 5972293 |
|
IMO changes to the C API should be in a separate PR, not combined with a 7000 line diff here. |
NVIDIA#1705 tracked the removed parameter enumeration and settings-file entry points as deferred work. mlubin pointed out that bindings can be generated from constants.h, which makes runtime enumeration unnecessary. That holds structurally, not just in practice: every one of the 117 parameter registrations in solver_settings.cu is written as a CUOPT_* macro from constants.h, and none of the 109 distinct registered names is absent from the header. The list get_parameter_names() returns is therefore derived from the same header the bindings generate from, and cannot contain anything extra. Loading and dumping a settings file is expressible on top of cuOptSetParameter and cuOptGetParameter, so that half needs no new C surface either. NVIDIA#1705 is closed. Point the README at the reasoning rather than describing the capability as pending. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 4eb70d8 |
|
@mlubin agreed — the C API changes are now #1715, off This PR is stacked on it. The Two related issues were also split out of the earlier discussion, so each is reviewable on its own:
#1705 is closed: you were right that runtime parameter enumeration is unnecessary. Every one of the 117 parameter registrations is written as a |
Documentation wording and the scoped solution guard in the C API tests, kept identical to the branch behind NVIDIA#1715 so this stack stays consistent until that PR merges and these files drop out of this diff. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test d981aec |
Keeps the cpp/ files in this stack identical to the branch behind NVIDIA#1715. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 6a04604 |
Adds two C API accessors so solver statistics are reachable from the C ABI, and closes #1202. Split out of #1524 at @mlubin's request, so the C API can be reviewed without the Java diff attached. The Java bindings are the first consumer but nothing here refers to them. ### The gap The C API reports the *outcome* of a solve — `cuOptGetTerminationStatus`, `cuOptGetObjectiveValue`, `cuOptGetMIPGap`, `cuOptGetSolutionBound`, `cuOptGetSolveTime`, and the primal/dual/reduced-cost arrays — but none of the diagnostics the C++ solution interfaces already carry. Of the eleven fields listed in #1202, **zero** are reachable today. Python sidesteps this by binding to the C++ structs through Cython, so the C ABI is a second-class path and every non-Python binding hits the same wall. Concretely, what these unlock: - **Judging a non-optimal answer.** When a solve stops on a time limit, residuals and gap are the only way to distinguish a nearly-converged solution from a useless one. - **Verifying a returned MIP solution.** The three violation magnitudes are what a caller checks before acting on a solution. - **Knowing which algorithm solved it.** Under `CUOPT_METHOD_CONCURRENT` the caller otherwise cannot tell PDLP from dual simplex. - **Performance work.** Iteration counts, nodes, simplex iterations, and presolve time. ### Why attributes rather than one getter per statistic #1202 proposed eleven individual getters. This uses the attribute model instead, following review feedback from @chris-maes: - it matches how problem data is already read (`cuOptGetProblemIntAttribute` and friends); - a future statistic is a new constant rather than a new exported symbol, so existing callers need no relink; - bindings that generate from `constants.h` — as the Java bindings already do for `CuOptConstants.java` — pick up new statistics with no hand-written code at all. Structured data stays on dedicated functions, which is the existing convention: `cuOptGetConstraintMatrix` is a function because CSR is three parallel arrays, and the same will apply to the quadratic constraint rows in #1703. The rule is scalars and homogeneous arrays as attributes, ragged or multi-output as functions. ### Safety Solution selectors are numbered in their own range (300+), so a problem selector passed to a solution accessor, or the reverse, is rejected rather than silently read. LP selectors require an LP solution and MIP selectors require a MIP solution, since the two come from different solvers; `CUOPT_ATTR_IS_MIP` on the originating problem says which set applies. The values are read straight off `lp_solution_interface_t` / `mip_solution_interface_t`, so this is exposure only — no computation, no solve-time cost. ### Tests `c_api.lp_solution_attributes` and `c_api.mip_solution_attributes` cover both solvers and the ways a caller can get it wrong: a float selector through the integer accessor and the reverse, the other solver's selectors, unknown selectors, and null arguments. Float outputs are seeded with `NaN` rather than a numeric sentinel, since the solver cannot legitimately produce `NaN` — so an accessor that never writes its output is caught, where a numeric sentinel would be indistinguishable from a real result. Verified locally: **69/69** `C_API_TEST` cases pass with the full LP/MIP/QP dataset. Authors: - Ramakrishna Prabhu (https://github.com/ramakrishnap-nv) Approvers: - Miles Lubin (https://github.com/mlubin) URL: #1715
|
/ok to test 8d854d3 |
Three of chris-maes's comments that stand on their own, ahead of the ones that depend on how far the API moves toward an attribute model. Remove IP from ProblemCategory. The engine still reports a third category for a problem whose variables are all discrete, so the mapping cannot simply be deleted: problem_category_t::IP is returned from cpu_optimization_problem.cpp when every variable is discrete, and fromNative would throw on it. The constant is gone from the public enum and the native value is folded into MIP instead, with a comment recording why the mapping exists. Consolidate file IO on read and write. readMPS forced the MPS parser where read already dispatches on the file extension, and Python has deprecated its equivalent, so the Java API should not be introducing the name. writeMPS is now write. This also removes the parseMPSProblem JNI entry point, which was the only caller of io::read_mps, so the bindings no longer reach into the parser for a second, redundant path. Rename getCSR and getQCSR. The review asked what a "linear objective matrix" was, and the answer is that the documentation was wrong: getCSR returns the constraint matrix, not an objective. They are now getConstraintMatrix and getQuadraticObjectiveMatrix, matching cuOptGetConstraintMatrix in the C API, and both carry a line saying what they return. Verified: 26/26 Java tests, 59 JNI symbols matched, all pre-commit hooks. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test dc8b4b0 |
The public enum constant is gone, so the paragraph documented an API that no longer exists. The only remaining mention is a comment in ProblemCategory explaining why the native value is folded into MIP, which has to stay while the engine still reports it. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
@chris-maes before I work through the rest of your review, I'd like to check the direction, because I think the remaining comments share one underlying question and answering it settles most of them at once. What's already done
On "fluent": @mlubin noted it is standard terminology for this style of API, so I have left the wording unless you would still prefer it changed. The questionThe Java surface was built for parity with the Python API — that was its design goal, and it is why the shape looks the way it does. Your review is asking it to be consistent with the C API instead, which took the attribute direction in #1549. Both are reasonable, but they point different ways, and five of the six items you have asked to remove exist in the Python API today:
So the choice is roughly: (a) Java aligns with the C API and diverges from Python. Java becomes the model the Python API moves toward later. Users moving between the two see different shapes until Python follows. (b) Java matches Python for now, and both move to an attribute model together as a separate piece of work.
What each remaining item would mean
"Attributes instead of stats" — this is the one I would most like pinned down, since it subsumes several of the above. Do you mean:
Option 2 is a much larger change and would be unusual for Java, where typed accessors are the norm and an untyped selector API gives up compile-time checking. Option 1 I can do now. Last thingThis PR is around 6,700 lines and has been open since July. Which of these do you consider blocking for merge, and which could land as follow-ups against the Java module once it is in? If most can follow, the bindings become reviewable on their own merits and the API refinement can proceed without holding up CI coverage and the build integration. |
getStatus already answers the question: it stays NO_TERMINATION until a solve populates it, so isSolved was a second way to ask something the API already told you. This is the one item from the review that needs no decision on the wider API direction, since the Python API has no equivalent either. The private solved field went with it, as isSolved was its only reader. Verified: 26/26 Java tests. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 8b178d9 |
Description
Java bindings at parity with the LP/MIP/QP surface, plus tests and docs.
The bindings are hand-written JNI over the public C API. Contributes to #1535 and #860.
Native surface
The bindings originally carried
cuopt_java_native_api.{hpp,cpp}, a Java-local shim declaring seven entry points thatlibcuoptdid not export. It reimplemented the memory layout of the opaquecuOptSolverSettingshandle in order to reach the underlyingsolver_settings_t, so any change tosolver_settings_handle_twould have silently corrupted it.All seven are now part of the public C API, implemented in
cuopt_c.cppbeside the functions they belong with:cuOptLoadParametersFromFile,cuOptDumpParametersToFile,cuOptGetNumSolverParameters,cuOptGetSolverParameterNamecuOptSolutionIsMIP,cuOptGetLPSolverStats,cuOptGetMIPSolverStatsThis covers the fields requested in #1202. Note for reviewers: #1202 proposed 11 individual getters and explicitly weighed that against a bulk call. This PR ships the bulk shape (two functions with nullable out-pointers), inherited from the original shim. If the individual-getter shape is preferred, say so and I will split them — flagging rather than silently closing #1202.
Build and CI
./build.sh javaand./build.sh java --run-java-tests— an opt-in target, matching how cuvs exposes its Java build. Preferscpp/buildso it works without--install, and falls back to the conda prefix, which is what CI uses.ci/build_java.shcallsbuild.shrather than duplicating the invocation.java/cuopt/scripts/test.shcarries aCUOPT_PRELOAD_LIBSescape hatch to work around [BUG] Build-tree libcuopt.so resolves librmm from the conda prefix instead of its own _deps, failing at dlopen #1704; it can be removed once that is fixed.release/26.08shared-workflow pins to@mainwithci-conda:26.10-latest, and the redundantcompute-matrixjob was dropped.Guard against JNI symbol drift
The bindings are hand-written, so a Java
static nativedeclaration and itsJava_com_nvidia_..._nameentry point can drift apart with nothing to catch it. JNI resolves methods lazily, so the library still loads and the failure only appears when something calls the method.java/cuopt/scripts/check_jni_symbols.shdiffs the prototypesjavac -hderives from the Java sources against the symbols the built library exports, and fails on a mismatch in either direction. It runs after every native build. It reads the built artifact rather than parsing source, so the macro-generated entry points need no special casing.Packaging
Version is
26.10.0— Maven has no notion of the zero-padded RAPIDS patch field, and26.10.00would sort oddly against a later26.10.1.update-version.shderives it. Sources and javadoc jars are attached, and the POM carries theurl,licenses,scm, anddevelopersmetadata Maven Central requires.Known gaps
These are deliberate, not oversights.
pdlp/cuopt_c_internal.hppfor the problem path — name setters, quadratic objective and quadratic constraint getters, and length reporting for duals and reduced costs. That coupleslibcuopt_jni.soto a specificlibcuoptbuild rather than to a stable ABI, which matters before this is published as a binary artifact. Tracked in [FEA] Expand cuopt_c.h with problem-model accessors for non-Python language bindings #1703.Checklist